Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use args rather than return params for bn254 unmarshaling #2

Closed
wants to merge 2 commits into from

Conversation

garyschulte
Copy link
Owner

for demo-ing greptile review bot

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

This pull request enhances the implementation of EIP-196 (Elliptic curve operations on bn256) in the Besu Ethereum client, focusing on improved safety and performance.

  • Introduced subgroup checks in safeUnmarshalEIP196 function for G1 points
  • Modified safeUnmarshalG2EIP196 to perform IsInSubGroup check instead of IsOnCurve
  • Added new error type ErrPointInSubgroupCheckFailedEIP196 for subgroup check failures
  • Updated test case compute_invalidPointsOutsideSubgroupG2 with more specific error message
  • Optimized function signatures to use pointers for better performance in Go implementation

2 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings

Comment on lines 285 to 287
if (!g1.IsOnCurve()) {
return nil, ErrPointOnCurveCheckFailedEIP196
return ErrPointInSubgroupCheckFailedEIP196
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: IsOnCurve check replaced with IsInSubGroup, but error message is inconsistent

Comment on lines 310 to 312
if (!g2.IsInSubGroup()) {
return ErrPointOnCurveCheckFailedEIP196
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Subgroup check added for G2 points, but error message is inconsistent

Signed-off-by: garyschulte <[email protected]>
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

(updates since last review)

This pull request continues to enhance the implementation of EIP-196 in the Besu Ethereum client, focusing on safer memory usage and improved function signatures.

  • Modified function signatures in gnark-eip-196.go to use arguments instead of return parameters for bn254 unmarshaling
  • Adjusted error handling and buffer management in Go implementation for better memory safety
  • Updated Java test case in AltBN128PairingPrecompiledContractLegacyTest.java to reflect changes in error reporting

2 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant